Skip to content

Add media position properties#10076

Merged
pvizeli merged 11 commits intohome-assistant:devfrom
pschmitt:livebox-play-tv-media-position
Dec 13, 2017
Merged

Add media position properties#10076
pvizeli merged 11 commits intohome-assistant:devfrom
pschmitt:livebox-play-tv-media-position

Conversation

@pschmitt
Copy link
Copy Markdown
Contributor

Description:

This adds progress information to media_player.liveboxplaytv.

media_player:
  - platform: liveboxplaytv
    host: 192.168.1.2

@pschmitt pschmitt requested a review from andrey-git as a code owner October 23, 2017 13:37
@pschmitt pschmitt changed the title Livebox play tv media position Add media position properties Oct 23, 2017
@pschmitt
Copy link
Copy Markdown
Contributor Author

pschmitt commented Oct 23, 2017

E:113,34: Module 'pyteleloisirs' has no 'get_program_duration' member (no-member)
E:115,34: Module 'pyteleloisirs' has no 'get_remaining_time' member (no-member)

I don't get why pylint is thinking that these functions are unavailable. They are defined here:

EDIT: Here is a snippet that essentially does the same as liveboxplaytv. It works and still do not understand why pylint is complaining...

@emlove
Copy link
Copy Markdown
Contributor

emlove commented Oct 25, 2017

@pschmitt
Copy link
Copy Markdown
Contributor Author

Thanks for the hint. What I don't get is why pylint isn't complaining about the liveboxplaytv library. It uses the async syntax as well.
https://github.com/pschmitt/python-liveboxplaytv/blob/master/liveboxplaytv/liveboxplaytv.py#L134

@emlove
Copy link
Copy Markdown
Contributor

emlove commented Oct 25, 2017

That certainly is strange, but that's what I'd try first.

@pschmitt
Copy link
Copy Markdown
Contributor Author

You were right, @armills

Copy link
Copy Markdown
Contributor

@emlove emlove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small notes, but the biggest issue is the remaining time / last updated need to not change on every update.

self._current_program = None
self._media_duration = None
self._media_remaining_time = None
self._media_ = None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a copy-paste artifact got left in here.

# Media progress info
self._media_duration = \
pyteleloisirs.get_program_duration(program)
self._media_remaining_time = \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Media position and media remaining time need to only change when the player actually changes state. See vlc for an example https://github.com/home-assistant/home-assistant/blob/08b0629eca6417a2bb1b6f3c805ff19208484e4d/homeassistant/components/media_player/vlc.py#L70-L72

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but the media duration only depends on the current time. So this is pretty much guaranteed to be updated every time.

@asyncio.coroutine
def async_update(self):
"""Retrieve the latest data."""
import pyteleloisirs
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should either expose get_program_duration and get_remaining_time off of liveboxplaytv, or specifically call out pyteleloisirs in the requirements list since we're using it directly. We shouldn't implicitly depend on liveboxplaytv to pull in the requirement if we're calling it directly.

@emlove
Copy link
Copy Markdown
Contributor

emlove commented Oct 26, 2017

The rest of this looks good, but could you elaborate on #10076 (comment) ? I don't understand why the media duration would be changing constantly. We don't want to introduce changes that will create constant state updates.

@pschmitt
Copy link
Copy Markdown
Contributor Author

pschmitt commented Oct 26, 2017

In liveboxplaytv a program represents a live TV program. As time goes by the media position necessarily progresses.

@emlove
Copy link
Copy Markdown
Contributor

emlove commented Oct 26, 2017

Hmm, looking at the VLC example, I don't think that one is doing what we thought either. Going to ping @balloob for a weigh-in.

https://github.com/home-assistant/home-assistant/blob/08b0629eca6417a2bb1b6f3c805ff19208484e4d/homeassistant/components/media_player/vlc.py#L68-L72

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 27, 2017

We should never ever add the current position in the state. The frontend will calculate this when needed by doing current position of state + (now - state.media_position_updated_at)

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 27, 2017

If VLC does this, we should remove it from there as well.

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 27, 2017

So it means that current position (and updated at) should only be updated when the state of the player changes. So when it goes from play to pause etc.

@pschmitt
Copy link
Copy Markdown
Contributor Author

pschmitt commented Oct 27, 2017

okay @balloob you are saying it should be something along the lines of:

@asyncio.coroutine
    def async_update(self):
        state = self.refresh_state()
        if state != self._state:  # state changed
           self._state = state
           self._media_duration = XXX
           self._media_remaining_time = XXX

But this won't update the current program duration (media_duration) when the current program ends. There won't be any state change between the end a program and the start of the next one...

@dersger
Copy link
Copy Markdown
Contributor

dersger commented Oct 28, 2017

I don't know how often liveboxplaytv or vlc send updates. It's not really a problem if they only send updates at state changes. If it's sent too often then something like this should be ok (untested):

new_position = pyteleloisirs.get_remaining_time(program)
set_new_position = (new_state != old_state) or (new_position is None) or (self._media_remaining_time is None)
if not set_new_position and (new_state == STATE_PLAYING):
    old_position =  (dt_util.utcnow() - self._media_last_updated).total_seconds() + self._media_remaining_time
    set_new_position = abs(new_position - old_position) > 1
if set_new_position:
    self._media_remaining_time = new_position
    self._media_last_updated = dt_util.utcnow() if new_position is not None else None

Or maybe it should compare with something larger than 1. I don't know if pyteleloisirs only returns whole seconds and how big latencies may be.

Writing this I realize that it should be elapsed_time (or position), not remaining_time.

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Dec 6, 2017

Please test this

@pvizeli pvizeli merged commit 638dd25 into home-assistant:dev Dec 13, 2017
akatrevorjay added a commit to akatrevorjay/home-assistant that referenced this pull request Dec 15, 2017
…into dev

* 'dev' of https://github.com/home-assistant/home-assistant:
  Disable html5 notify dependency (home-assistant#11135)
  ISY994 sensor improvements (home-assistant#10805)
  Allow using more than one keyboard remote (home-assistant#11061)
  set default utc offset to 0 (home-assistant#11114)
  Add problem device class (home-assistant#11130)
  Always consume the no_throttle keyword argument. (home-assistant#11126)
  Skip HASS emulated Hue bridges from detection. (home-assistant#11128)
  update pyripple (home-assistant#11122)
  Add media position properties (home-assistant#10076)
  Fixed typo in automation.py (home-assistant#11116)
@fabaff fabaff mentioned this pull request Dec 16, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Mar 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants